-
Notifications
You must be signed in to change notification settings - Fork 0
chore(metrics): Refactor current metrics #185
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: evalir/fix-flashbots-blocking
Are you sure you want to change the base?
Conversation
We're not following our set pattern for metrics (see node-components) and it makes it hard to know what we're really tracking and where.
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
| counter!("signet.builder.built_blocks").increment(1); | ||
| histogram!("signet.builder.built_blocks.tx_count").record(built_block.tx_count() as u32); | ||
| metrics::built_blocks().increment(1); | ||
| metrics::built_blocks_tx_count().record(built_block.tx_count() as u32); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these lines should both be collapsed into metrcis::record_bulit_block(&block) and the logic to increment, etc should be in the metrics file
prestwich
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is lowkey a NACK from me rn. this work predates our current metrics pattern. the new work doesn't follow our pattern, and adds metrics we don't necessariy want. is there a design doc for this I'm not seeing?
| /// Logs a debug message if the channel is closed. | ||
| fn track_outbound_tx(&self, envelope: &alloy::consensus::TxEnvelope) { | ||
| counter!("signet.builder.flashbots.").increment(1); | ||
| metrics::flashbots_submissions().increment(1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto here. we don't want the caller to be responsible for the .increment(1), as it requires knowing the metric semantics
| // Don't submit empty blocks | ||
| if sim_result.block.is_empty() { | ||
| counter!("signet.builder.flashbots.empty_block").increment(1); | ||
| metrics::flashbots_empty_blocks().increment(1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
| let result = | ||
| self.prepare(&sim_result).instrument(span.clone()).await.inspect_err(|error| { | ||
| counter!("signet.builder.flashbots.bundle_prep_failures").increment(1); | ||
| metrics::flashbots_bundle_prep_failures().increment(1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
| .instrument(submit_span.clone()) | ||
| .await; | ||
|
|
||
| metrics::flashbots_submission_duration_ms() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
| match response { | ||
| Ok(resp) => { | ||
| counter!("signet.builder.flashbots.bundles_submitted").increment(1); | ||
| metrics::flashbots_bundles_submitted().increment(1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
| .inspect(|_| { | ||
| metrics::quincey_signature_duration_ms() | ||
| .record(start.elapsed().as_millis() as f64); | ||
| metrics::quincey_signatures().increment(1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
| .inspect_err(|_| { | ||
| counter!("signet.builder.quincey_signature_failures").increment(1) | ||
| metrics::quincey_signature_duration_ms() | ||
| .record(start.elapsed().as_millis() as f64); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
| counter!("signet.builder.quincey_signature_failures").increment(1) | ||
| metrics::quincey_signature_duration_ms() | ||
| .record(start.elapsed().as_millis() as f64); | ||
| metrics::quincey_signature_failures().increment(1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
| .provider | ||
| .get_transaction_count(self.provider.default_signer_address()) | ||
| .into_future() | ||
| .instrument(debug_span!("nonce_fetch")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what exactly is this span doing? it records no info and has no real parent
| .into_future() | ||
| .instrument(debug_span!("nonce_fetch")) | ||
| .await?; | ||
| metrics::nonce_fetch_duration_ms().record(start.elapsed().as_millis() as f64); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is this metric for? why do we want to track this?
| // record how long it took to mine the transaction | ||
| // potential improvement: use the block timestamp to calculate the time elapsed | ||
| histogram!("metrics.tx_mine_time").record(start.elapsed().as_millis() as f64); | ||
| metrics::tx_mine_time_ms().record(start.elapsed().as_millis() as f64); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
| // log whether the transaction reverted | ||
| if receipt.status() { | ||
| counter!("metrics.tx_succeeded").increment(1); | ||
| metrics::tx_succeeded().increment(1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
| debug!("tx succeeded"); | ||
| } else { | ||
| counter!("metrics.tx_reverted").increment(1); | ||
| metrics::tx_reverted().increment(1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
| Err(PendingTransactionError::TxWatcher(WatchTxError::Timeout)) => { | ||
| // log that the transaction timed out | ||
| counter!("metrics.tx_not_mined").increment(1); | ||
| metrics::tx_not_mined().increment(1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
| } | ||
| Err(e) => { | ||
| counter!("metrics.rpc_error").increment(1); | ||
| metrics::rpc_error().increment(1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto

We're not following our set pattern for metrics (see node-components) and it makes it hard to know what we're really tracking and where.